-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace global fetch patch with Turbo-specific behavior #1025
Replace global fetch patch with Turbo-specific behavior #1025
Conversation
This commit introduces the concept of page refresh. A page refresh happens when Turbo renders the current page again. We will offer two new options to control behavior when a page refresh happens: - The method used to update the page: with a new option to use morphing (Turbo currently replaces the body). - The scroll strategy: with a new option to keep it (Turbo currently resets scroll to the top-left). The combination of morphing and scroll-keeping results in smoother updates that keep the screen state. For example, this will keep both horizontal and vertical scroll, the focus, the text selection, CSS transition states, etc. We will also introduce a new turbo stream action that, when broadcasted, will request a page refresh. This will offer a simplified alternative to fine-grained broadcasted turbo-stream actions. Co-Authored-By: Jorge Manrubia <jorge@hey.com>
7ac1faf
to
d1935bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @seanpdoyle,
Thanks for working on this. What I had in mind here was two things:
- Define an internal
fetch
that decorateswindow.fetch
with the new behavior, without overriding it. Turbo will internally use this internalfetch
. And applications will be able to import it too:
import { fetch } from "@hotwired/turbo-rails"
- Expose it also via
Turbo.window
, so that libraries that doesn't build against Turbo – such as requestjs-rails – can still do something like:
const fetch = window.Turbo?.fetch || window.fetch
(1) will be the preferred form of using the new fetch from apps. (2) is intended a fallback mechanism.
I think your refactor is a nice improvement over the original PR, but it's kind of misaligned with (1). I think we need a custom fetch
to use internally and export to clients, instead of making the current requests observable.
What do you think?
@jorgemanrubia I think I am misunderstanding the use case for applications to utilize a Turbo-specific Why would an application need to make a request with an If they're submitting a request outside of |
As an alternative to patching `fetch` globally, this commit introduces a new `FetchRequestObserver` class, and makes the `Session` instance a delegate. As prep-work, this commit also moves the `recentRequests` out of the global `@hotwired/turbo` module an into an instance property on `Session`. The `FetchRequestObserver` instance attaches an event listener for `turbo:before-fetch-request` (in this case, on the `<html>` element). Instead of generating the request ID in the delegate, this commit also extends the `FetchRequest` class to generate the `uuid()` and set it to the new `FetchRequest.id` property. The `Session.willFetch` delegate method will be invoked before each request, and will read the `X-Turbo-Request-Id` header and write it to the instance's `recentRequests` set.
328c93a
to
6c318e2
Compare
@seanpdoyle because that way the application makes sure that the request id is registered by Practical case: you have a Drag & Drop system that relies on In the general case, you are interested in all the broadcasted "page refreshes" except the ones that you originate. The vast majority of your page refreshes will come via Turbo actions, but not necessarily all of them, since using |
Ahhh, so the header is less about fetch's integration with HTTP, and more about guarding against background broadcasts over Web Sockets. That's something that was missing from my perspective on the problem. With that in mind, is there a difference in outcomes between Turbo support for something like: import { fetch } from "@hotwired/turbo"
fetch("/my-path", { ... }) versus something like: import { withTurboHeaders } from "@hotwired/turbo"
window.fetch("/my-path", withTurboHeaders({ ... }))
// or maybe
import { FetchRequest } from "@hotwired/turbo"
window.fetch("/my-path", FetchRequest.prepare({ ... })) Both scenarios require awareness that Turbo does something special with handling |
@seanpdoyle not in outcomes but in ergonomics from the caller side, imo. Also, the first form's encapsulation is higher, hiding away the fact that internally it's manipulating the request headers. |
Hello Thanks for all the work going on, it looks really promising. I wanted to share a few thoughts: import { fetch } from "@hotwired/turbo" I personally think having a custom fetch option coming from Turbo will add lots of confusion at the end. I have been really enjoying the @rails/request.js library so far. The API is nice with lots of small goodies. Out of all the fetch libraries that I have been using, I think it has the best API. in #1019 (comment) @marcelolx proposed some help to make Rails/request.js Turbo compatible. My favorite solution would be to be able to use this package for all my fetch-related actions. Therefore project NOT using @rails/request.js, I would second @seanpdoyle suggestion for something like that. import { withTurboHeaders } from "@hotwired/turbo"
window.fetch("/my-path", withTurboHeaders({ ... })) This makes it clear that it is not the preferred option. The preferred option would be documented by using request.js. And for Rails/request.js let's make sure this works out of the box from the standard fetch library provided by Rails. |
Hey @adrienpoly, +1 to make I don't agree with making the not-preferred option less ergonomic though. |
The issue I see with exposing this new custom fetch from Turbo as a public API is that it will make it harder down the road to unify Turbo and Request.js. To me, it should remain a private API inside Turbo or come from request.js When I saw Request.js for the first time, I saw it as an extraction of Turbo internal fetch and thought the plan was to ultimately replace Turbo internal fetch with request.js. I am sure there are quite a few differences in both implementations but last time I looked there was also a lot of duplication in both code bases. IMHO, the most comprehensive API would be import { get, put, post, delete } from "@hotwired/turbo" and those get, put, post, and delete would be coming from request.js |
9125652
to
00b3015
Compare
Closing due to comments above: #1025 (review). |
As an alternative to patching
fetch
globally, this commit introduces anew
FetchRequestObserver
class, and makes theSession
instance adelegate.
As prep-work, this commit also moves the
recentRequests
out of theglobal
@hotwired/turbo
module an into an instance property onSession
.The
FetchRequestObserver
instance attaches an event listener forturbo:before-fetch-request
(in this case, on the<html>
element).Instead of generating the request ID in the delegate, this commit also
extends the
FetchRequest
class to generate theuuid()
and set it tothe new
FetchRequest.id
property.The
Session.willFetch
delegate method will be invoked before eachrequest, and will read the
X-Turbo-Request-Id
header and write it tothe instance's
recentRequests
set.